-
Notifications
You must be signed in to change notification settings - Fork 39
sdk(llm): Stop model-name whack-a-mole: revert to core family substring matching #879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…normalize usage - model_matches now does case-insensitive substring on full raw model - strip trailing '*' in patterns (migration aid) - pattern tables converted to plain substrings (no '*') - drop normalize_model_name and related tests - update tests to reflect substring semantics and Bedrock coverage Fixes #844 Co-authored-by: openhands <[email protected]>
…handling and empty-token skipping - Patterns are now used exactly as provided (lowercased/stripped) - No special handling for '*' or empty tokens Co-authored-by: openhands <[email protected]>
…eature detection - Validate provider-prefixed Bedrock ids and plain vendor-prefixed names - Ensure function-calling and prompt-cache features are enabled for Claude families Co-authored-by: openhands <[email protected]>
…edrock dotted vendor prefixes - Function-calling: adds claude-sonnet-4-5 and claude-sonnet-4.5, and us.anthropic.* examples - Prompt cache: keep only supported families; drop unsupported haiku-4.5 dotted vendor case Co-authored-by: openhands <[email protected]>
… extend tests with dotted vendor forms - Add claude-haiku-4.5 and claude-haiku-4-5 to PROMPT_CACHE_PATTERNS - Expand tests for us.anthropic.* and local names for Haiku 4.5 Co-authored-by: openhands <[email protected]>
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
2 similar comments
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
This comment was marked as outdated.
This comment was marked as outdated.
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
@OpenHands Merge main into this PR and fix the conflicts. |
|
Uh oh! There was an unexpected error starting the job :( |
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
🧪 Integration Tests ResultsOverall Success Rate: 100.0% 📁 Detailed Logs & ArtifactsClick the links below to access detailed agent/LLM logs showing the complete reasoning process for each model. On the GitHub Actions page, scroll down to the 'Artifacts' section to download the logs.
📊 Summary
📋 Detailed Resultslitellm_proxy_claude_sonnet_4_5_20250929
litellm_proxy_gpt_5_mini_2025_08_07
litellm_proxy_deepseek_deepseek_chat
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! thank you
This updates the direct completion access to use resp.message and extract TextContent, aligning with the current LLMResponse interface. Co-authored-by: openhands <[email protected]>
…ring matching semantics and add function_calling support flag - Keep substring-based model_matches per PR #879 direction - Restore function calling patterns and features - Align tests to substring semantics and function_calling expectations Co-authored-by: openhands <[email protected]>
…ing semantics - Remove supports_function_calling field and patterns - Remove related tests - Keep substring-based model_matches; keep other feature flags intact Co-authored-by: openhands <[email protected]>
Co-authored-by: openhands <[email protected]>
…mini-latest' and gpt-5 family Co-authored-by: openhands <[email protected]>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
@xingyaoww I'll fix the conflicts etc, but I'd love your attention for a bit here: this PR simplifies things in agent-sdk right now, because we actually lost the recognition of the "claude-x" in model names, when the provider had a more complex name than just "anthropic/". In other words, we lost bedrock match. This fixes it.
The cause for that is that we changed at some point in the recent past, from simple match of the core name ("claude-3-5") anywhere in the full provider/model (which is accurate! No Llama will call itself "claude-3-5" 😅), to some code using globbing normalization which forced us to add half a million patterns to account for the variety of forms in which "claude-3-5" is included out there - and it keeps missing some, of course.
I looked into it and I believe that was a mistake I made. This is a revert.
This fixes some reports on slack on Bedrock. I'd love to merge this, to stop the whack-a-mole, rather than hardcoding more stuff in these patterns. I think maybe we can also take them out, but I would still love this revert first.
Summary
What & Why
This PR restores the durable invariant:
=> the feature applies.
"bedrock/anthropic.claude-3-5-sonnet-20241022-v2:0"=>"claude-3-5-sonnet"applies.This eliminates the pattern maintenance whack-a-mole caused by dotted prefixes and provider-specific suffixes and aligns again with proven behavior in the wild.
Recent refactor introduced fnmatch-based globbing over a normalized basename. This unintentionally diverged from the prior V0 behavior where we effectively matched by substring on the full provider/model name. That change broke real-world cases, notably with AWS Bedrock where names embed dotted vendor prefixes and version suffixes inside the basename (e.g., bedrock/anthropic.claude-3-5-sonnet-20241022-v2:0). Our patterns like 'claude-3-5-sonnet*' stopped matching after normalization and globbing.
Implementation Details
model_matches()
Pattern tables: remove '*'
normalize_model_name()
Tests
Outcomes
Checklist
Closes #844
@enyst can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
golang:1.21-bookwormeclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22Pull (multi-arch manifest)
Run
All tags pushed for this build
The
45b3ecetag is a multi-arch manifest (amd64/arm64); your client pulls the right arch automatically.Cross-repo impact: Fix: OpenHands/OpenHands#11248